SolveRoot with local Optimization by amirroth · Pull Request #11429 · NatLabRockies/EnergyPlus

amirroth · 2026-02-23T17:38:25Z

Pull request overview

This PR introduces a wrapper to SolveRoot that attempts to select the optimal algortihm (RegulaFalsi, Bisection, SomeCombinationA, AnotherCombinationB) on a call site basis.

It introduces function SolveRoot2 which takes an additional solveRootStats structure reference argument. It uses this argument to run a different candidate algorithm during each call. After every candidate algorithm has run five times, the winning algorithm (i.e., the one that resulted in the fewest total iterations) is used for the rest of the run. There is a new unit test that demonstrates how the function is supposed to work.

The SolveRootStats structure should be part of state to allow it to be shared across calls. It could be part of module state if all objects of the same type should share a single algorithm. It should be part of object state if each object is optimized independently.

There are of course a number of small diffs in the regression suite, and these are due to the fact that SolveRoot may be using a different algorithm and is therefore coming up with slightly different roots. Most of the diffs are "small". But even the "big" diffs are relatively small. Is a temperature difference of 0.005 C "big"? Is a power difference of 0.1 W "big"? Right now we have a fixed definition where an absolute difference of 0.001 or a relative difference of 0.005 is considered big. But maybe we need to rethink things. Maybe we need to ratchet up the absolute difference globally, or maybe selectively (this would be more complicated).

Right now, this new function is only used in five modules (SingleDuct, WaterThermalTanks, and several others), and never in sizing routines, which presumably only run once. I will slowly propagate it in future PRs. Others feel free to do the same.

amirroth

Short code review, not much to this PR.

amirroth · 2026-02-23T17:40:24Z

src/EnergyPlus/Autosizing/WaterHeatingCoilUASizing.cc

                    this->dataErrorsFound = true;
-                } else if (SolFla == -2) {
+
+                } else if (SolFla == General::SOLVEROOT_ERROR_INIT) {


Added these constexpr definitions as well since nobody can be expected to remember what -1 and -2 stand for.

I don't think I'll ever forget

amirroth · 2026-02-23T17:41:07Z

src/EnergyPlus/DataStringGlobals.cc

@@ -0,0 +1,71 @@
+// EnergyPlus, Copyright (c) 1996-2024, The Board of Trustees of the University of Illinois,


I don't know what this is doing here.

Should these files be removed?

Looks like these two files may have been added in inadvertently. I went ahead and removed them.

amirroth · 2026-02-23T17:41:33Z

src/EnergyPlus/General.cc

    XRes = XTemp;
 }

+// A second version that does not require a payload -- use lambdas


This is the new function.

amirroth · 2026-02-23T17:42:55Z

src/EnergyPlus/General.cc

+    RootAlgo algoTemp = state.dataRootFinder->rootAlgo;
+    state.dataRootFinder->rootAlgo = stats.algo;
+
+    SolveRoot(state, Eps, maxIters, SolFla, XRes, f, X_0, X_1);


At some point, the algortihm should just be made a parameter to this call, but that would have resulted in 200+ diffs.

amirroth · 2026-02-23T17:43:54Z

src/EnergyPlus/HVACSystemRootFindingAlgorithm.hh

-};
 struct RootFindingData : BaseGlobalStruct
 {
-    HVACSystemRootFindingAlgorithm HVACSystemRootFinding;


No need for this additional object when RootFindingData is already an enclosing object.

amirroth · 2026-02-23T17:46:43Z

src/EnergyPlus/WaterCoils.cc

    auto f = [&state, H, RH, PB](Real64 const Tprov) { return H - Psychrometrics::PsyHFnTdbRhPb(state, Tprov, RH, PB); };

-    General::SolveRoot(state, Acc, MaxIte, SolFla, Tprov, f, T0, T1);
+    static General::SolveRootStats solveRootStats;


This is not thread safe, but I didn't now where to put it. Someplace in dataPsychrometrics I guess?

Maybe here:

EnergyPlus/src/EnergyPlus/General.hh

Line 310 in ee54360

struct GeneralData : BaseGlobalStruct

amirroth · 2026-02-23T17:47:32Z

third_party/kiva/cmake/TargetArch.cmake

@@ -5,12 +5,23 @@
 # Regarding POWER/PowerPC, just as is noted in the Qt source,


Not sure why this is here. Probably needs a separate PR if someone hasn't already addressed this.

amirroth · 2026-02-23T17:49:26Z

tst/EnergyPlus/unit/AirTerminalSingleDuctMixer.unit.cc

    EXPECT_NEAR(state->dataLoopNodes->Node(thisFanCoil.ATMixerPriNode).MassFlowRate, 0.1, 0.000001);
-    EXPECT_NEAR(state->dataLoopNodes->Node(thisFanCoil.ATMixerSecNode).MassFlowRate, 0.035129, 0.000001);
-    EXPECT_NEAR(state->dataLoopNodes->Node(thisFanCoil.ATMixerOutNode).MassFlowRate, 0.135129, 0.000001);
+    EXPECT_NEAR(state->dataLoopNodes->Node(thisFanCoil.ATMixerSecNode).MassFlowRate, 0.035129, 0.00003);


Since we are changing root finding algorithm, answers may differ slightly. There is no reason why tolerances should be 0.0001%. Nothing in BEM is known to this level of precision.

amirroth · 2026-02-23T17:50:45Z

tst/EnergyPlus/unit/General.unit.cc

 }

+TEST_F(EnergyPlusFixture, General_SolveRoot2)
+{


New unit test to make sure SolveRoot2 works as expected. Use two kinds of functions.

amirroth · 2026-02-23T17:52:40Z

tst/EnergyPlus/unit/WaterThermalTanks.unit.cc

    EXPECT_TRUE(WaterThermalTanks::TankOperatingMode::Floating == HeatPump.Mode); // expect HP to switch to floating mode since it reached set point
-    EXPECT_NEAR(58.0005664, Tank.TankTempAvg, 0.0000001);                         // average tank temp over time step
-    EXPECT_NEAR(58.0005664, Tank.SourceOutletTemp, 0.0000001);                    // source outlet = average tank temp
+    EXPECT_NEAR(58.0005664, Tank.TankTempAvg, 0.001); // Was 0.0000001 !?!    // average tank temp over time step


Why do temperatures need tolerances of 0.0000001 C? Isn't 0.001 C enough?

Our temperature "sensors" are perfect, so yes ;)

github-actions · 2026-02-23T18:25:38Z

⚠️ Regressions detected on macos-14 for commit `a1fbf48`

Regression Summary

ERR: 4
Table Small Diffs: 9
EIO: 10
ESO Small Diffs: 16
MTR Small Diffs: 14
Table Big Diffs: 4
Table String Diffs: 7
ESO Big Diffs: 4

github-actions · 2026-02-23T18:31:25Z

⚠️ Regressions detected on ubuntu-24.04 for commit `a1fbf48`

Regression Summary

ERR: 4
Table Small Diffs: 7
EIO: 11
ESO Small Diffs: 16
MTR Small Diffs: 13
Table Big Diffs: 4
Table String Diffs: 7
ESO Big Diffs: 4
MTR Big Diffs: 1

github-actions · 2026-02-23T20:36:01Z

⚠️ Regressions detected on ubuntu-24.04 for commit `67f212e`

Regression Summary

ERR: 5
Table Small Diffs: 12
EIO: 11
ESO Small Diffs: 15
MTR Small Diffs: 14
Table Big Diffs: 2
Table String Diffs: 7
ESO Big Diffs: 5

github-actions · 2026-02-23T20:36:26Z

⚠️ Regressions detected on macos-14 for commit `67f212e`

Regression Summary

ERR: 5
Table Small Diffs: 11
EIO: 9
ESO Small Diffs: 15
MTR Small Diffs: 14
Table Big Diffs: 3
Table String Diffs: 6
ESO Big Diffs: 5

rraustad · 2026-02-23T23:53:21Z

Just a sanity check on what happens with this branch. No real diff, operation changed because of convergence difference.

github-actions · 2026-02-24T00:52:32Z

⚠️ Regressions detected on ubuntu-24.04 for commit `14199e3`

Regression Summary

ERR: 5
Table Small Diffs: 12
EIO: 11
ESO Small Diffs: 15
MTR Small Diffs: 14
Table Big Diffs: 2
Table String Diffs: 7
ESO Big Diffs: 5

github-actions · 2026-02-24T00:56:15Z

⚠️ Regressions detected on macos-14 for commit `14199e3`

Regression Summary

ERR: 5
Table Small Diffs: 11
EIO: 9
ESO Small Diffs: 15
MTR Small Diffs: 14
Table Big Diffs: 3
Table String Diffs: 6
ESO Big Diffs: 5

amirroth · 2026-02-24T18:25:50Z

Just a sanity check on what happens with this branch. No real diff, operation changed because of convergence difference.

Thanks @rraustad.

github-actions · 2026-02-24T19:12:15Z

⚠️ Regressions detected on macos-14 for commit `0fa27ec`

Regression Summary

ERR: 5
Table Small Diffs: 11
EIO: 9
ESO Small Diffs: 15
MTR Small Diffs: 14
Table Big Diffs: 3
Table String Diffs: 6
ESO Big Diffs: 5

github-actions · 2026-02-24T19:31:46Z

⚠️ Regressions detected on ubuntu-24.04 for commit `0fa27ec`

Regression Summary

ERR: 5
Table Small Diffs: 12
EIO: 11
ESO Small Diffs: 15
MTR Small Diffs: 14
Table Big Diffs: 2
Table String Diffs: 7
ESO Big Diffs: 5

github-actions · 2026-02-24T19:38:47Z

⚠️ Regressions detected on ubuntu-24.04 for commit `4f81891`

Regression Summary

ERR: 5
Table Small Diffs: 12
EIO: 11
ESO Small Diffs: 15
MTR Small Diffs: 14
Table Big Diffs: 2
Table String Diffs: 7
ESO Big Diffs: 5

github-actions · 2026-02-24T20:28:05Z

⚠️ Regressions detected on macos-14 for commit `4f81891`

Regression Summary

ERR: 5
Table Small Diffs: 11
EIO: 9
ESO Small Diffs: 15
MTR Small Diffs: 14
Table Big Diffs: 3
Table String Diffs: 6
ESO Big Diffs: 5

mitchute

No major questions

mitchute · 2026-02-24T22:21:49Z

src/EnergyPlus/General.cc

+#ifdef GET_OUT
+        if (NIte > 20) {
+            assert(false);
+            Flag = NIte;
+            XRes = XTemp;
+            return;
+        }
+#endif // GET_OUT
+


Is this something you intend to leave in?

mitchute · 2026-02-24T22:28:20Z

src/EnergyPlus/SingleDuct.cc

            if (QTotLoad < QHeatFanOffMax - SmallLoad) {
                // vary HW flow, leave air flow at minimum
-                ErrTolerance = this->ControllerOffset;
+                ErrTolerance = this->ControllerOffset / 2; // Added /2 to try to eliminate a "big" diff


Clean this out?

mitchute · 2026-02-24T22:42:36Z

src/EnergyPlus/WaterCoils.cc

    auto f = [&state, H, RH, PB](Real64 const Tprov) { return H - Psychrometrics::PsyHFnTdbRhPb(state, Tprov, RH, PB); };

-    General::SolveRoot(state, Acc, MaxIte, SolFla, Tprov, f, T0, T1);
+    static General::SolveRootStats solveRootStats;


Maybe here:

EnergyPlus/src/EnergyPlus/General.hh

Line 310 in ee54360

struct GeneralData : BaseGlobalStruct

mitchute · 2026-02-24T22:46:08Z

src/EnergyPlus/WaterThermalTanks.cc

+                            // Shared by all instances of this call to learn fastest algorithm.  This is not
+                            // thread, "object", or "state" safe, does it really need to be?  Does it need to
+                            // exist within state?  Within each thermal tank object?  That's probably excessive.
+


Agreed. I can't think of why this wouldn't work. We wouldn't really expect different instances of the same coil to have different algorithms, right?

mitchute · 2026-02-24T22:47:24Z

src/EnergyPlus/WaterThermalTanks.cc


    int constexpr MaxIte(500);   // maximum number of iterations
-    Real64 constexpr Acc(0.001); // Accuracy of result from RegulaFalsi
+    Real64 constexpr Acc(0.005); // Accuracy of result from RegulaFalsi // was 0.001, trying to eliminate some "big" diffs


Seems OK. (cleanup comment?)

mitchute · 2026-02-24T22:49:54Z

src/EnergyPlus/WaterToAirHeatPumpSimple.cc

                        if (PltSizNum > 0) {
                            DesignEntWaterTemp = state.dataSize->PlantSizData(PltSizNum).ExitTemp;
-                            ratioTS = (DesignEntWaterTemp + state.dataWaterToAirHeatPumpSimple->CelsiustoKelvin) / Tref;
+                            ratioTS = (DesignEntWaterTemp + Constant::Kelvin) / Tref;


That's fun. How many layers of constexpr can we go here.

mitchute · 2026-02-24T22:54:42Z

tst/EnergyPlus/unit/WaterThermalTanks.unit.cc

    EXPECT_TRUE(WaterThermalTanks::TankOperatingMode::Floating == HeatPump.Mode); // expect HP to switch to floating mode since it reached set point
-    EXPECT_NEAR(58.0005664, Tank.TankTempAvg, 0.0000001);                         // average tank temp over time step
-    EXPECT_NEAR(58.0005664, Tank.SourceOutletTemp, 0.0000001);                    // source outlet = average tank temp
+    EXPECT_NEAR(58.0005664, Tank.TankTempAvg, 0.001); // Was 0.0000001 !?!    // average tank temp over time step


Our temperature "sensors" are perfect, so yes ;)

Amir Roth and others added 9 commits November 2, 2025 18:16

Merge develop; resolve conflicts; new approach

85f05f7

Merge develop; resolve conflicts

9d88281

Merge develop; resolve conflicts

52dea61

Oops

bedec29

Propagate to a few more modules, revert some WAHP changes

34819ea

Merge remote-tracking branch 'origin/develop' into SolveRoot

ecb0d1e

Merge develop

833a468

Move state to objects (mostly) for thread-safety

f9c4ccb

Merge remote-tracking branch 'origin/develop' into SolveRoot

429ed2e

amirroth requested review from mitchute and rraustad February 23, 2026 17:38

amirroth self-assigned this Feb 23, 2026

amirroth added the DoNotPublish Includes changes that shouldn't be reported in the changelog label Feb 23, 2026

amirroth added this to the EnergyPlus 26.1 milestone Feb 23, 2026

amirroth commented Feb 23, 2026

View reviewed changes

Trying to eliminate some "big" diffs

391fbf5

Merge remote-tracking branch 'origin/develop' into SolveRoot

7bbec98

Merge branch 'develop' into SolveRoot

b978dc8

remove unused files

a09442a

mitchute reviewed Feb 24, 2026

View reviewed changes

		@@ -0,0 +1,71 @@
		// EnergyPlus, Copyright (c) 1996-2024, The Board of Trustees of the University of Illinois,

		@@ -5,12 +5,23 @@
		# Regarding POWER/PowerPC, just as is noted in the Qt source,

Conversation

amirroth commented Feb 23, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pull request overview

Uh oh!

amirroth left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented Feb 23, 2026

⚠️ Regressions detected on macos-14 for commit a1fbf48

Uh oh!

github-actions bot commented Feb 23, 2026

⚠️ Regressions detected on ubuntu-24.04 for commit a1fbf48

Uh oh!

github-actions bot commented Feb 23, 2026

⚠️ Regressions detected on ubuntu-24.04 for commit 67f212e

Uh oh!

github-actions bot commented Feb 23, 2026

⚠️ Regressions detected on macos-14 for commit 67f212e

Uh oh!

rraustad commented Feb 23, 2026

Uh oh!

github-actions bot commented Feb 24, 2026

⚠️ Regressions detected on ubuntu-24.04 for commit 14199e3

Uh oh!

github-actions bot commented Feb 24, 2026

⚠️ Regressions detected on macos-14 for commit 14199e3

Uh oh!

amirroth commented Feb 24, 2026

Uh oh!

github-actions bot commented Feb 24, 2026

⚠️ Regressions detected on macos-14 for commit 0fa27ec

Uh oh!

github-actions bot commented Feb 24, 2026

⚠️ Regressions detected on ubuntu-24.04 for commit 0fa27ec

Uh oh!

github-actions bot commented Feb 24, 2026

⚠️ Regressions detected on ubuntu-24.04 for commit 4f81891

Uh oh!

github-actions bot commented Feb 24, 2026

⚠️ Regressions detected on macos-14 for commit 4f81891

Uh oh!

mitchute left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

amirroth commented Feb 23, 2026 •

edited

Loading

⚠️ Regressions detected on macos-14 for commit `a1fbf48`

⚠️ Regressions detected on ubuntu-24.04 for commit `a1fbf48`

⚠️ Regressions detected on ubuntu-24.04 for commit `67f212e`

⚠️ Regressions detected on macos-14 for commit `67f212e`

⚠️ Regressions detected on ubuntu-24.04 for commit `14199e3`

⚠️ Regressions detected on macos-14 for commit `14199e3`

⚠️ Regressions detected on macos-14 for commit `0fa27ec`

⚠️ Regressions detected on ubuntu-24.04 for commit `0fa27ec`

⚠️ Regressions detected on ubuntu-24.04 for commit `4f81891`

⚠️ Regressions detected on macos-14 for commit `4f81891`